Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java] Fix SubClass annotations missing from the base class #4085

Merged
merged 3 commits into from
Nov 21, 2016

Conversation

szakrewsky
Copy link
Contributor

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

#2449

@wing328
Copy link
Contributor

wing328 commented Nov 1, 2016

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Contributor

wing328 commented Nov 1, 2016

Please also run ./bin/java-petstore-all.sh to update Java Petstore samples if you've not done so.

@wing328 wing328 added this to the v2.2.2 milestone Nov 1, 2016
@szakrewsky
Copy link
Contributor Author

@wing328 I did run ./bin/java-petstore-all.sh I guess I was confused whether I was supposed to commit those file or if it was just running some tests.

@wing328
Copy link
Contributor

wing328 commented Nov 1, 2016

@szakrewsky please commit the update Java Petstore files as well so that CIs can test the change.

@szakrewsky
Copy link
Contributor Author

@wing328 Thanks. I think I found some issues, this commit only effects jersey but it seems that the other frameworks are adding the annotations as well. I will fix first.

@szakrewsky szakrewsky force-pushed the jackson-subtypes branch 2 times, most recently from 7bd38fd to 2e61550 Compare November 2, 2016 15:11
@szakrewsky
Copy link
Contributor Author

@wing328 I believe this is ready now. Let me know if there is anything more I need to do.

@wing328
Copy link
Contributor

wing328 commented Nov 2, 2016

@szakrewsky I'll review and let you know if I've any questions.


@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "className" )
@JsonSubTypes({
@JsonSubTypes.Type(value = Dog.class, name = "Dog"),@JsonSubTypes.Type(value = Cat.class, name = "Cat"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szakrewsky Can we use 2-space instead of 2 tabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@jeff9finger
Copy link
Contributor

The PR looks to only place the annotations on the first level of inheritance. For example:

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: sub_obj_type
        required:
          - sub_obj_type
        properties:          
          sub_obj_type:
            $ref: '#/definitions/SubObjType'
          name:
            type: string
            readOnly: true
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

In this case, the Jackson annotations are not placed on the SubObj class

@jeff9finger
Copy link
Contributor

jeff9finger commented Nov 17, 2016

Adding the following to DefaultCodeGen seems to fix the issue for me. However, this assumes that allOf: can have no more than one inline element that contains a discriminator. (My experience also shows that allOf: understands no more than 2 inline elements. If more than to exist, only last one is recognized).

@@ -1240,6 +1251,12 @@ public class DefaultCodegen {
                 allProperties = new LinkedHashMap<String, Property>();
                 allRequired = new ArrayList<String>();
                 m.allVars = new ArrayList<CodegenProperty>();
+                for (Model innerModel: composed.getAllOf()) {
+                    if (innerModel instanceof ModelImpl) {
+                        m.discriminator = ((ModelImpl) innerModel).getDiscriminator();
+                        break; // only one discriminator allowed at the moment.
+                    }
+                }
             } else {
                 allProperties = null;
                 allRequired = null;

@willgorman
Copy link

I'm interested in this same feature but for generated JAX-RS server code as well as Java clients. It looks like that works by applying the same changes to the templates in the JavaJaxRS resource directory. Would it be possible to add that here or should I submit a separate PR after this is merged?

@szakrewsky
Copy link
Contributor Author

@jeff9finger I just fixed one issue which I had already documented before, #2449 (comment)

In your example the following code would have been generated,

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj") })
public class BaseObj {
}

now you will get

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj"),  @Type(value = DailySubObj.class, name = "DailySubObj")})
public class BaseObj {
}

The annotations will be added as long as the discriminator value is set, why that only happens for ModelImpl and not ComposedModel I don't know, but I would probably say that is a different issue.

@jeff9finger
Copy link
Contributor

jeff9finger commented Nov 18, 2016

The generated code above is not correct. It might work, but does not represent the actual structure of the inheritance. I need the annotations to be on the SubObj class. Like so:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "object_type")
@JsonSubTypes({ @Type(value = SubObj.class, name = "SubObj") })
public class BaseObj {
}

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "sub_obj_type")
@JsonSubTypes({ @Type(value = DailySubObj.class, name = "DailySubObj") })
public class SubObj {
}

Notice that there is a different discriminator on SubObj - "sub_obj_type". This is completely lost unless the @JsonTypeInfo annotations are on the SubObj class.

"discriminator" is only available on ModelImpl, so until the swagger spec is updated to handle discriminator in more than one object, it has to live there. That is why the discriminator field needs to be populated for any object that has it declared in the swagger spec.

Without the code above, or something similar, any sub objects that declare a discriminator, will not have the discriminator set in the ModelImpl object in the code that represents the swagger definition. And this is why the @JsonTypeInfo annotations are not placed on the sub class.

@willgorman You should be able to simply add the mustache templates to the other Java based templates to get these annotations. I'll need them eventually in a jaxrs server and in inflector.

@jeff9finger
Copy link
Contributor

@szakrewosky (I'm new to git) How can I merge your code into my local master repository? I don't see a branch that the commits for this issue are in. Thanks

@wing328 wing328 merged commit 7696559 into swagger-api:master Nov 21, 2016
@wing328
Copy link
Contributor

wing328 commented Nov 21, 2016

@jeff9finger the PR has been merged into master. Please pull the latest to give it a try.

@szakrewsky thanks for your contribution.

@jeff9finger
Copy link
Contributor

The implementation of the sub-sub class does not work in my case.

See my comment above: #4085 (comment)

In my data model (and probably other people's as well), this approach is wrong. I don't want to have all the @JsonSubType.@Type annotations on the base class because I need to discriminate sub-sub classes differently that the sub class

I think my comment referenced explains this. It seems to me that making an assumption about sub-sub (and deeper) classes must have more options on the approach than simply assuming that all the sub-sub classes will be serialized as one type of object.

@szakrewsky
Copy link
Contributor Author

@jeff9finger As far as I know, if Jackson is deserializing BaseObj and you want it to instantiate the proper subclass to have working polymorphism, "My" solution is correct. DailySubObj is indeed a subclass of BaseObj even if it is a few levels down in the hierarchy. If you also want Jackson to deserialize the SubObj with proper polymorphism, then "Your" solution is also correct, however at the current time I think it depends on the separate issue of the discriminator not being set #4226. Fix that, and you will get what you want, if I understand your problem correctly.

@jeff9finger
Copy link
Contributor

jeff9finger commented Nov 21, 2016

@szakrewsky Thanks for your comments. Let me amend my statement from above.
Instead of "In my data model (and probably other people's as well), this approach is wrong." I should have said "In my data model (and probably other people's as well), this approach is not desired."

So, I agree with you, that the solution is correct, but with a caveat. In my case, I have the annotation regarding DailySubObj on BaseObj, but it is not strictly needed for serialization (as long as it is defined on the SubObj) - though I don't think it causes any harm. I'll be looking into this to verify.

The annotation for the sub-sub class is not desired because in some cases (mine to be specific) there are many sub-sub classes and I prefer that they not pollute the base class when they are not strictly required.

I am suggesting that there be an option to specifying the @JsonSubType.@Type annotation for sub-sub (and deeper) classes on the base object. This can definitely be handled in a new issue though, so this one can be closed. What do you think about that?

@jeff9finger
Copy link
Contributor

@szakrewsky What does the spec look like when there are sub sub classes in your case? Doesn't there always need to be a discriminator for inheritance? Just wondering why the cm is always added to the parent.parent.children. The parent would need to have a discriminator defined right?

In other words, how can a sub sub class be defined in a swagger wprc without the sub class specifying a discriminator? (The only way I can think that it would make sense is if the sub class defined the same discriminator property as the base class.

@szakrewsky
Copy link
Contributor Author

@jeff9finger The spec that exhibits this problem is the same as your spec just without the discriminator on the SubObj. Now what does this mean? It could mean that DailySubObj is actually composed of SubObj and therefore SubObj doesn't exist as a class at all and there is only one level of inheritance. However, it appears that swagger-code-gen will generate a class for SubObj that extends BaseObj, and a class for DailySubObj that extends SubObj.

I did a little more research and it appears that the JsonSubTypes annotation is recursive. So you could put the JsonTypeInfo annotation at the top level of the inheritance hierarchy, and have the JsonSubTypes annotation only include direct subclasses. The JsonSubType annotation would then be included recursively down the inheritance tree for subclasses that themselves have subclasses. Would this solve your problem? What exactly is your problem by the way? Is it just a preference over annotation placement or do you get a stacktrace while deserializing something?

@jeff9finger
Copy link
Contributor

The open api spec specifies that discriminator is required for inheritance.

Here is the spec that shows SubObj without a discriminator.

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        properties:          
          name:
            type: string
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

You state "... it appears that swagger-code-gen will generate a class for SubObj that extends BaseObj, and a class for DailySubObj that extends SubObj."

This indeed appears to be the case.

However, this is not the behavior that I am specifying in my original spec. I've included it below for clarity.

definitions:
BaseObj:
    type: object
    discriminator: object_type
    required:
      - id
      - object_type
    properties:
      id:
        type: integer
        format: int64
      object_type:
        type: string
        readOnly: true
  SubObjType:
    type: string
    enum: 
    - daily
    - monthly
    - quarterly
    - yearly

  SubObj:
    allOf:
      - $ref: '#/definitions/BaseObj'
      - type: object
        discriminator: sub_obj_type
        required:
          - sub_obj_type
        properties:          
          sub_obj_type:
            $ref: '#/definitions/SubObjType'
          name:
            type: string
            readOnly: true
  DailySubObj:
    allOf:
      - $ref: '#/definitions/SubObj'
      - type: object
         properties:
           day_of_month:
             type: integer
             format: int32
             readOnly: true

The annotations on SubObj require a different discriminator property. Therefore it must be represented on that class, otherwise the desired inheritance is lost - i.e The discriminator property would be completely lost if the the Jackson annotations are placed on BaseObj.

Notice that the discriminator for BaseObj is "object_type", but the discriminator for SubObj is "sub_obj_type". These are distinctions that are required in my data model.

So, if all the annotations for DailySubObj are placed on BaseObj (using the discriminator property "object_type"), I'd lose the discriminator ("sub_obj_type") and related inheritance for DailySubObj.

While Jackson handles the annotations recursively, doing so in this case does not accurately represent the semantics of the open api spec.

See issue #4226 for more detail. My suggestion there is to only add the children of an object to the parent.parent.children if there is no discriminator.

@jeff9finger
Copy link
Contributor

@wing328 Please take a look at this pr as well. It is related to #4252

@wing328
Copy link
Contributor

wing328 commented Jan 9, 2017

@jeff9finger this PR was merged into master back on Nov 21, 2016 via the commit 7696559

@jeff9finger
Copy link
Contributor

OK. I have another change required to handle the issues discussed here. Will open a pr for that. Is a new issue created?

@wing328 wing328 changed the title Issue #2449 SubClass annotations are missing from the base class [Java] Fix SubClass annotations missing from the base class Feb 20, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
… class (swagger-api#4085)

* petstore up to latest

* Issue swagger-api#2449 SubClass annotations are missing from the base class

* include child in all its super types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants